-
Notifications
You must be signed in to change notification settings - Fork 9
Tests for multi choice value #2872
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
- SMProAssayCreateTest.testAddResultFields - SMSampleCreateTest.testSamplesWithMultiChoice - SMSourceTypeMultiChoiceTest - ListTest.testMultiChoiceValues
…ag() fix test testMultiChoiceEditInBulk()
-add new shuffleSelect -replace my method with shuffleSelect
labkey-danield
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few comments.
| if(textChoiceValidator.getMultipleSelections()) | ||
| { | ||
| fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only set it if textChoiceValidator.getMultipleSelections is true, correct? This is ok if you are creating a field, but wouldn't work as expected if the test is editing an existing field and wants to change to field from a multi-choice to a single choice.
Perhaps a better check would be:
if(null != textChoiceValidator.getMultipleSelections())
| lookupSelect.clearSelection(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this work if a test is trying to add to an existing selections?
| if(textChoiceValidator.getMultipleSelections()) | ||
| { | ||
| fieldRow.setAllowMultipleSelections(textChoiceValidator.getMultipleSelections()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would make it impossible to disable multi-select through this method.
This should either call setAllowMultipleSelections unconditionally or the condition should check whether textChoiceValidator.getMultipleSelections() != null.
| /** | ||
| * Select a single facet value by clicking its label. Should replace all existing selections. | ||
| * @param value desired value | ||
| */ | ||
| public void selectFilter(String value) | ||
| { | ||
| elementCache().filterTypeSelects.select(value); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this comment to describe this method (looks leftover from the selectValue). The comment should also mention that this is only relevant to multi-value text choice fields.
This could also take a Filter.Operator instead of a String for some extra type-safety.
| List<String> values = (List<String>) value; | ||
| filterModal.selectFacetTab().selectValue(values.get(0)); | ||
| filterModal.selectFacetTab().checkValues(values.toArray(String[]::new)); | ||
| filterModal.selectFacetTab().selectFilter(operator.getDisplayValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will (or could) break functionality for other column types; the facet panel usually doesn't have a filter type.
Also, this wouldn't work for "Is Empty" or "Is Not Empty".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m planning to add support for 'Is Empty' and 'Is Not Empty' as well.
Regarding the functionality for other types: I’ve already found one test that was failing, so I added a check to the initFilterColumn method. It now checks if the filter type selector is present on the page before trying to interact with it. After this change, the test is passing.
Does this approach sound okay to you?
| for (Map.Entry<String, String> entry : values.entrySet()) | ||
| { | ||
| WebElement fieldInput = Locator.name(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); | ||
| WebElement fieldInput = Locator.nameContaining(EscapeUtil.getFormFieldName(entry.getKey())).findElement(getDriver()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a more precise locator and add a comment explaining why this can't match the name exactly. (See UpdateQueryRowPage)
| table.clickInsertNewRow(); | ||
| String valuesToChoose = tcValues.subList(1, 3).stream() | ||
| .sorted() | ||
| .collect(Collectors.joining(" ")); | ||
| Locator loc = Locator.nameContaining(EscapeUtil.getFormFieldName(columnName)); | ||
| selectOptionByText(loc, valuesToChoose); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clickInsertNewRow returns a page object that should have methods for setting fields.
| String encodedListName = "multiChoiceList"; | ||
| String keyName = "'><script>alert(\":(\")</script>'"; | ||
| String columnName = "MultiChoiceField"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using randomly generated field and list names. (I'm not sure whether ListHelper can handle the tricky characters but it's worth a try)
| String encodedListName = "multiChoiceList"; | |
| String keyName = "'><script>alert(\":(\")</script>'"; | |
| String columnName = "MultiChoiceField"; | |
| String encodedListName = TestDataGenerator.randomDomainName("multiChoiceList", DomainUtils.DomainKind.IntList); | |
| String keyName = TestDataGenerator.randomFieldName("'><script>alert(\":(\")</script>'"); | |
| String columnName = TestDataGenerator.randomFieldName("MultiChoiceField"); |
| selectOptionByText(loc, valuesToChoose); | ||
|
|
||
| clickButton("Submit"); | ||
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checker() doesn't take screenshots automatically. If you're doing a single check, checker().withScreenshot().verifyX will trigger a failure screenshot. If you're doing several checks, you should call checker().screenShotIfNewError(); after the checks.
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); | |
| checker().withScreenshot().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); |
| clickButton("Submit"); | ||
|
|
||
| // verify the multi choice value is persisted | ||
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a screenshot.
| checker().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); | |
| checker().withScreenshot().verifyEquals("Multi choice value not as expected", valuesToChoose, table.getDataAsText(0, columnName)); |
| if(Boolean.parseBoolean(selectElement.getAttribute("multiple"))) { | ||
| List<WebElement> elems = selectElement.findElements(Locator.tag("option")); | ||
| elems.forEach(element->{ | ||
| if(value.contains(element.getAttribute("value")) ^ element.isSelected()) element.click(); | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less precise than we like for broadly available helpers; it would get confused by selection options that have overlapping text. I would suggest having a separate method for multi-select (selectOptionsByText(WebElement selectElement, List<String> values).
| public void testMultiChoiceValues() | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only run the test on Postgres.
| public void testMultiChoiceValues() | |
| { | |
| public void testMultiChoiceValues() | |
| { | |
| Assume.assumeTrue("Multi-choice text fields are only supported on PostgreSQL", WebTestHelper.getDatabaseType() == WebTestHelper.DatabaseType.PostgreSQL); |
Co-authored-by: Trey Chadick <tchad@labkey.com>
- Test fixes for change from quoted values to bold in error messages
getConversionErrorMessage() fix to message case for invalid date/timestamp now quoting field name for consistency
Rationale
Some changes to support multi choice
Related Pull Requests
Changes